fix(services): derive commandArgv when an editor sends only the text command - #574
Closed
AbdullahM07 wants to merge 1 commit into
Closed
fix(services): derive commandArgv when an editor sends only the text command#574AbdullahM07 wants to merge 1 commit into
AbdullahM07 wants to merge 1 commit into
Conversation
…command oblien#332 made `commandArgv` the argv Openship hands Docker, keeping `["sh","-c",<command>]` only as the fallback for legacy rows that have no argv. Neither service editor derives it, and the dashboard's settings form sends the one-line `command` field alone, so the two columns drift apart. updateService writes the new `command` and leaves the row's PREVIOUS argv in place. `toComposeSpec` backfills only a null argv, so the stale one survives the deploy-time syncFromCompose and reconcileFromCompose keeps "ours" for an edited row: the container goes on running the old command while the UI shows the new one, clearing the field never restores the image CMD, and composeSpecDiff reports phantom `commandArgv` drift on a service the operator edited to match its own compose file. createService stores a text command with a null argv — the app installer forwards a template's string command through here. That null IS covered by the deploy-time backfill, so it is not a live regression; derived at the insert anyway so the row is written consistently rather than depending on a repair step. Derive the argv alongside the existing trim in both editors. An explicitly-sent argv stays authoritative, `[]` included, so the compose parser, the CLI and a snapshot replay are never second-guessed and a quoted `sh -c "…"` argv is never re-split. Routing-only callers are untouched — the update path keys off `"command" in patch`. Three of the four new assertions fail on main (stale argv, uncleared argv, null argv on create) and pass with this change. `tsc --noEmit` clean for apps/api. The file's pre-existing Prettier drift is left alone. Closes oblien#573
Member
Author
|
Superseded — Upstream's version is the better one, so there is nothing worth salvaging here:
Same for the tests. Closing rather than rebasing — a rebase here would only re-litigate a fix that already landed in a stronger form. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #573.
What was broken
#332 made
commandArgvthe argv Openship hands Docker, keeping["sh","-c",<command>]only as the fallback for legacy rows that have no argv at all. Neither service editor derives that argv, and the dashboard's settings form sends the one-linecommandfield alone — so the two columns drift apart:updateServicewrites the newcommandand leaves the row's previous argv in place (patch = { ...data },commandgetstrimOrNull, andrepos.service.updateis a rawset).toComposeSpecbackfills only a null argv, so the stale one survives the deploy-timesyncFromComposeandreconcileFromComposekeeps "ours" for an edited row. The container goes on running the old command while the UI shows the new one, clearing the field never restores the imageCMD, andcomposeSpecDiffreports phantomcommandArgvdrift on a service the operator edited to match its own compose file.createServicestores a text command withcommandArgv: null— the app installer forwards a template's string command through here (13 catalog services declare one). That null is covered by the backfill, so it is not a livesh -cregression today; I tracedbuild-pipeline.ts→syncFromCompose→ the pipeline's freshlistByProjectread and confirmed the first deploy repairs it. Included anyway so the row is written consistently instead of depending on a repair step.The compose-file path was already correct —
shellSplitWords,commandToArgvandresolveComposeCmdall behave as documented. This is only the API write path.The change
Derive the argv from the text
commandin both editors when the caller didn't send one — inupdateServicealongside the existing trim, so the two columns can't be written out of sync, and increateServiceat the insert. An explicitly-sentcommandArgvstays authoritative,[](clear the imageCMD) included, so argv-aware callers — the compose parser, the CLI, a snapshot replay — are never second-guessed and a quotedsh -c "…"argv is never re-split.Two derivation lines plus a regression test; no schema, endpoint or runtime change.
How I verified it
apps/api/test/modules/services/service-command-argv.test.tsdrives the realcreateService/updateServiceagainst mocked repos and asserts the patch that reaches the DB.On
main, 3 of the 4 assertions fail:With the change, all 4 pass. The fourth passes both ways on purpose — it pins the boundary, since the tempting wrong implementation (always re-split the text) would mangle an explicit
["sh","-c","echo hi && ak server"].Also run:
bun run --cwd apps/api test— 328/333 files pass on the branch. The 5 that fail are pre-existing on this machine and unrelated: the same suite on cleanmainfails 6 files (a superset), always the whole-app route/permission builders (app.routes,collection-project-routes,env-reveal-gating,pending-actions-tools,service-exec-isolation) pluscontainer-events, all on 10–20s timeouts, and one surfacingINTERNAL_TOKEN is required when DEPLOY_MODE="docker"from my local.env. Their membership shifts run to run while the diff stays constant.test/modules/services/— the module this change touches,service-exec-isolationincluded — passes 5/5 files, 42 tests, plus the 4 new ones.bun run --cwd apps/api lint—tsc --noEmitclean.bunx prettier --checkon both touched files. The new test file is clean.service.service.tswas already Prettier-dirty onmain(54 hunks — long single-line imports), so per CONTRIBUTING I left that drift alone rather than reformatting lines this change doesn't need; the added lines match the surrounding style.Not in this PR
entrypoint:is still listed inUNSUPPORTED_SERVICE_KEYS— a compose file that overrides the entrypoint gets a warning and is then ignored. Noted in #573 as a separate gap.